Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve textual notation of Call targets. #49

Merged
merged 1 commit into from
Mar 16, 2020
Merged

Improve textual notation of Call targets. #49

merged 1 commit into from
Mar 16, 2020

Conversation

vext01
Copy link
Contributor

@vext01 vext01 commented Mar 13, 2020

The textual notation for a Call target is kind of weird.

If there's a known symbol, it looks like this:

call operand=sym_name=_ZN50_$LT$T$u20$as$u20$core..convert..Into$LT$U$GT$$GT$4into17hcac79ab93d213ca6E, ret_bb=bb23

I think operand=sym_name= looks weird.

This change kills sym_name= and in case of an unknown target uses <unknown> (instead of unknown).

@ptersilie
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Mar 13, 2020
@bors @vext01
Merge #49
6e57191 
49: Improve textual notation of Call targets. r=ptersilie a=vext01

The textual notation for a Call target is kind of weird.

If there's a known symbol, it looks like this:
```
call operand=sym_name=_ZN50_$LT$T$u20$as$u20$core..convert..Into$LT$U$GT$$GT$4into17hcac79ab93d213ca6E, ret_bb=bb23
```

I think `operand=sym_name=` looks weird.

This change kills `sym_name=` and in case of an unknown target uses `<unknown>` (instead of `unknown`).



Co-authored-by: Edd Barrett <vext01@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 13, 2020

Build failed

@vext01
Copy link
Contributor Author

vext01 commented Mar 13, 2020

thread 'main' panicked at 'Cannot run dynamic test fn out-of-process', src/libtest/lib.rs:497:22

That's new...

@bjorn3
Copy link
Collaborator

bjorn3 commented Mar 13, 2020

-Zpanic-abort-tests was recently enabled in ykrustc.

@vext01
Copy link
Contributor Author

vext01 commented Mar 14, 2020

-Zpanic-abort-tests was recently enabled in ykrustc.

Yeah, I wondered if it might be related to that.

Next week I'll be investigating. At this point I'm not sure what a "dynamic test" is :P

@bjorn3
Copy link
Collaborator

bjorn3 commented Mar 14, 2020

Dynamic tests are for example used by compiletest_rs. Basically a dynamic test can be a closure, while a static test must be a function pointer. I can't find where you are using a dynamic test in ykrt though.

@vext01
Copy link
Contributor Author

vext01 commented Mar 16, 2020

I can confirm that the failure is caused by the abort strategy.

The failing tests are the benchmark tests marked #[bench]. Comment those and the failures go away...

@vext01
Copy link
Contributor Author

vext01 commented Mar 16, 2020

And here's the problem: you cannot do benchmark tests with the abort strategy
rust-lang/cargo#3166

It looks like normal tests and benchmark tests get mixed into the same binary, so we can't build just benchmark tests with the unwind strategy and the others with the abort strategy. And besides, we probably do want to allow benchmarks of our tracer.

So we might have to honour the unwind strategy after all.

@ptersilie @ltratt any thoughts on this?

vext01 added a commit to vext01/ykrustc that referenced this pull request Mar 16, 2020
@vext01
Revert "Use the abort strategy when tracing."
7479076 
This reverts commit c8c964a.

The reason for this is that benchmark tests require the unwind strategy and
since benchmarks are mixed with normal tests in the same binary, it means that
all of our tests would have to use the unwind strategy.

See:
ykjit/yk#49 (comment)
@vext01 vext01 mentioned this pull request Mar 16, 2020
Merged
bors bot added a commit to softdevteam/ykrustc that referenced this pull request Mar 16, 2020
@bors @vext01
Merge #92
d5fa5ab 
92: Revert "Use the abort strategy when tracing." r=ptersilie a=vext01

This reverts commit c8c964a.

The reason for this is that benchmark tests require the unwind strategy and
since benchmarks are mixed with normal tests in the same binary, it means that
all of our tests would have to use the unwind strategy.

See:
ykjit/yk#49 (comment)

Co-authored-by: Edd Barrett <vext01@gmail.com>
@vext01
Copy link
Contributor Author

vext01 commented Mar 16, 2020

I've reverted the switch to the abort strategy for now.

bors try

bors bot added a commit that referenced this pull request Mar 16, 2020
@bors
Try #49:
785dafd
@bors
Copy link
Contributor

bors bot commented Mar 16, 2020

try

Build succeeded

@vext01
Copy link
Contributor Author

vext01 commented Mar 16, 2020

@ptersilie this is ready for merge.

@ptersilie
Copy link
Contributor

bors r+

@vext01
Copy link
Contributor Author

vext01 commented Mar 16, 2020

bors ping

@bors
Copy link
Contributor

bors bot commented Mar 16, 2020

pong

@ptersilie
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2020

Build succeeded

@bors bors bot merged commit 5f42f8d into ykjit:master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ptersilie ptersilie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vext01 @ptersilie @bjorn3